Normalize strings transformation #371

binh-dam-ibigroup · 2021-03-22T15:24:13Z

Checklist

Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
[na] The description lists all applicable issues this PR seeks to resolve
The description lists any configuration setting(s) that differ from the default settings
All tests and CI builds passing

Description

This PR adds functionality for normalizing strings in a GTFS table:

Capitalize strings, with exceptions
Substitute text/words, with optional space normalization around words, if substituting words.
In addition:
ZipTransform and DbTransform are refactored to pass the correct FeedTransformTarget type to the implementers
Parameter validation is extracted from the transformation code itself.

Two optional configuration parameters are introduced:

DEFAULT_CAPITALIZATION_EXCEPTIONS, a comma-separated list of words as they should appear after capitalization (acronyms, cardinal points)
DEFAULT_SUBSTITUTIONS, a comma-separated list of text and its replacement, in the format old => new, or old =>+ new if space needs to be normalized around new.

Things that have yet to be resolved:

There is a "convert to sentence case" stub/option in NormalizeFieldTransformation, is it important?
Should we support for the GTFS table files inside a folder in the GTFS ZIP archive? e.g. gtfs.zip/google_transit/routes.txt

…tests.

… space normalization)

…rentheses.

…tests.

… space normalization)

…rentheses.

…n exceptions.

…t zip).

…' into normalize-strings-transformation # Conflicts: # src/main/java/com/conveyal/datatools/manager/models/transform/NormalizeFieldTransformation.java # src/test/java/com/conveyal/datatools/manager/models/transform/NormalizeFieldTransformationTest.java

…ametrized tests.

…, add cap exceptions.

…ased.

…ialize excep/substitutions.

codecov-io · 2021-03-31T23:13:12Z

Codecov Report

Merging #371 (e03367f) into dev (f0a3183) will increase coverage by 0.92%.
The diff coverage is 67.73%.

@@             Coverage Diff              @@
##                dev     #371      +/-   ##
============================================
+ Coverage     26.61%   27.53%   +0.92%     
- Complexity      649      688      +39     
============================================
  Files           178      180       +2     
  Lines          9188     9354     +166     
  Branches       1243     1260      +17     
============================================
+ Hits           2445     2576     +131     
- Misses         6504     6533      +29     
- Partials        239      245       +6

Flag	Coverage Δ	Complexity Δ
unit_tests	`27.53% <67.73%> (+0.92%)`	`688.00 <47.00> (+39.00)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
.../conveyal/datatools/manager/models/CustomFile.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...ls/manager/models/transform/ZipTransformation.java	`35.29% <31.25%> (-64.71%)`	`6.00 <5.00> (+5.00)`	⬇️
...com/conveyal/datatools/manager/jobs/DeployJob.java	`18.31% <47.25%> (+0.69%)`	`24.00 <14.00> (+8.00)`
...ols/manager/models/transform/DbTransformation.java	`53.12% <51.61%> (-46.88%)`	`4.00 <3.00> (+3.00)`	⬇️
...ransform/ReplaceFileFromVersionTransformation.java	`82.75% <88.88%> (+16.09%)`	`6.00 <3.00> (+2.00)`
...models/transform/NormalizeFieldTransformation.java	`90.00% <90.00%> (ø)`	`21.00 <21.00> (?)`
.../datatools/manager/jobs/ArbitraryTransformJob.java	`91.66% <100.00%> (ø)`	`3.00 <0.00> (ø)`
...eyal/datatools/manager/jobs/OtpRunnerManifest.java	`100.00% <100.00%> (ø)`	`1.00 <0.00> (ø)`
.../conveyal/datatools/manager/models/Deployment.java	`4.09% <100.00%> (+0.96%)`	`4.00 <0.00> (-1.00)`	⬆️
.../models/transform/DeleteRecordsTransformation.java	`74.19% <100.00%> (+12.79%)`	`5.00 <0.00> (-1.00)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0a3183...e03367f. Read the comment docs.

…desired case.

…e tests pass)

…oolsTest.

…e for the test.

…read sleep.

…e and thread sleep." This reverts commit a62e14e.

… data to zip file

…StringBuilder

…ing zip lastModified.

src/main/java/com/conveyal/datatools/manager/models/transform/TypedFeedTransformation.java

src/main/java/com/conveyal/datatools/common/status/MonitorableJob.java

evansiroky

In general, I approve overall of this PR. However, I think we may want to rethink whether we should have the transform rules be a list within the FeedSource object. I think it deserves its own mongo collection because it is probably unnecessary to fetch all transformation data whenever a feed source is fetched. Also, it might also make sense to combine the FeedTransformRules, FeedRetrievalMethod and FeedTransformation classes together into a single FeedTransformation class. These design decisions were made prior to this PR, but I think moving forward, we should seriously consider refactoring this and avoid making additional lists within model classes unless a very small amount of data is expected within these lists.

evansiroky

Sorry, just seeing a failing build and my comment about a PR with a proposed refactor is now changing my mind to request changes.

…on-eas Combine TypedFeedTransformation and FeedTransformation

binh-dam-ibigroup · 2021-05-05T15:32:46Z

In general, I approve overall of this PR. However, I think we may want to rethink whether we should have the transform rules be a list within the FeedSource object. I think it deserves its own mongo collection because it is probably unnecessary to fetch all transformation data whenever a feed source is fetched. Also, it might also make sense to combine the FeedTransformRules, FeedRetrievalMethod and FeedTransformation classes together into a single FeedTransformation class. These design decisions were made prior to this PR, but I think moving forward, we should seriously consider refactoring this and avoid making additional lists within model classes unless a very small amount of data is expected within these lists.

I have created issue #380 with your comments above.

br648

Minor comments, but looks good.

br648 · 2021-05-11T11:25:07Z

src/main/java/com/conveyal/datatools/manager/models/transform/DeleteRecordsTransformation.java

+
+    @Override
+    public void validateParameters(MonitorableJob.Status status) {
+        // Does nothing.


Is this method needed then?

The method is needed because validateParameters is now abstract in the parent class. Is @Override needed though?

src/main/java/com/conveyal/datatools/manager/models/transform/NormalizeFieldTransformation.java

…ss Javadoc.

configurations/test/server.yml.tmp

Co-authored-by: Landon Reed <[email protected]>

…rmSubstitutions'

src/main/java/com/conveyal/datatools/manager/models/transform/NormalizeFieldTransformation.java

…NormalizeFieldTransformation.java Co-authored-by: Landon Reed <[email protected]>

landonreed and others added 17 commits February 19, 2021 17:16

feat(normalize-strings): Add normalize string transform for GTFS

1383053

refactor(NormalizeFieldTransformation.java): add some fields

b3b1134

refactor(NormalizeFieldTransformation): Fix capitalize function. Add …

3de77bf

…tests.

refactor(NormalizeFieldTransformation): Add string replacement (basic…

f3aaa18

… space normalization)

refactor(NormalizeFieldTransformation): Add removal of contents in pa…

6a52f5b

…rentheses.

feat(normalize-strings): Add normalize string transform for GTFS

0f9b6f6

refactor(NormalizeFieldTransformation.java): add some fields

fb038a1

refactor(NormalizeFieldTransformation): Fix capitalize function. Add …

1616199

…tests.

refactor(NormalizeFieldTransformation): Add string replacement (basic…

f3c1e99

… space normalization)

refactor(NormalizeFieldTransformation): Add removal of contents in pa…

76643d3

…rentheses.

refactor(NormalizeFieldTransformation): Add support for capitalizatio…

4d6c02f

…n exceptions.

refactor(NormalizeFieldTransformation): Add output to zip (assume fla…

0a92484

…t zip).

refactor(NormalizeFieldTransformationTest): Convert unit tests to par…

c5a926c

…ametrized tests.

refactor(NormalizeFieldTransformation): Make transform steps optional…

342cf65

…, add cap exceptions.

refactor(NormalizeFieldTransformation): Make substitutions instance-b…

f38fd40

…ased.

refactor(NormalizeFieldTransformation): Use persisted strings to init…

330decb

…ialize excep/substitutions.

binh-dam-ibigroup added 12 commits April 1, 2021 16:14

refactor(NormalizeFieldTransformation): Tweak code.

65f8545

test(NormalizeFieldTransformation): Add basic e2e test

086a599

refactor(NormalizeFieldTransformation): Define cap exceptions in the …

3965c05

…desired case.

refactor(NormalizeFieldTransformJobTest): Extract new test class (mak…

c4b6161

…e tests pass)

refactor(NormalizeFieldTransformJobTest): Remove setup code

ff541d0

refactor(NormalizeFieldTransformJobTest): Make test derive from Datat…

0b0f363

…oolsTest.

refactor(NormalizeFieldTransformJobTest): Persist feedsource only onc…

d28e000

…e for the test.

refactor(NormalizeFieldTransformJobTest): Try a test duplicate and th…

a62e14e

…read sleep.

Revert "refactor(NormalizeFieldTransformJobTest): Try a test duplicat…

80a725c

…e and thread sleep." This reverts commit a62e14e.

refactor(NormalizeFieldTransformation): Try a delay writing processed…

5d5482a

… data to zip file

refactor(NormalizeFieldTransformation): Use CsvListWriter instead of …

b1aa23f

…StringBuilder

refactor(NormalizeFieldTransformation): Replace Thread.sleep by touch…

a3a3c54

…ing zip lastModified.

binh-dam-ibigroup assigned evansiroky May 4, 2021

refactor: combine FeedTransformation and TypedFeedTransformation

bcfa8a7

evansiroky reviewed May 5, 2021

View reviewed changes

src/main/java/com/conveyal/datatools/manager/models/transform/TypedFeedTransformation.java Outdated Show resolved Hide resolved

evansiroky reviewed May 5, 2021

View reviewed changes

src/main/java/com/conveyal/datatools/common/status/MonitorableJob.java Show resolved Hide resolved

evansiroky approved these changes May 5, 2021

View reviewed changes

evansiroky suggested changes May 5, 2021

View reviewed changes

evansiroky assigned binh-dam-ibigroup and unassigned evansiroky May 5, 2021

Merge pull request #379 from ibi-group/normalize-strings-transformati…

678639e

…on-eas Combine TypedFeedTransformation and FeedTransformation

binh-dam-ibigroup mentioned this pull request May 5, 2021

Refactor FeedSource, FeedTransformRules and FeedRetrievalMethod #380

Open

binh-dam-ibigroup requested a review from evansiroky May 5, 2021 15:38

binh-dam-ibigroup assigned evansiroky and unassigned binh-dam-ibigroup May 5, 2021

br648 approved these changes May 11, 2021

View reviewed changes

br648 removed their assignment May 11, 2021

refactor(NormalizeFieldTransformation): Remove unused import, fix cla…

7bd9975

…ss Javadoc.

landonreed reviewed May 13, 2021

View reviewed changes

configurations/test/server.yml.tmp Outdated Show resolved Hide resolved

evansiroky approved these changes May 13, 2021

View reviewed changes

evansiroky removed their assignment May 13, 2021

binh-dam-ibigroup and others added 2 commits May 14, 2021 14:59

Update configurations/test/server.yml.tmp

97c760d

Co-authored-by: Landon Reed <[email protected]>

refactor(NormalizeFieldTransformation): Remove redundant field 'perfo…

6fb9a0f

…rmSubstitutions'

binh-dam-ibigroup requested a review from landonreed May 14, 2021 22:40

landonreed reviewed May 25, 2021

View reviewed changes

src/main/java/com/conveyal/datatools/manager/models/transform/NormalizeFieldTransformation.java Outdated Show resolved Hide resolved

binh-dam-ibigroup and others added 2 commits May 25, 2021 14:05

Update src/main/java/com/conveyal/datatools/manager/models/transform/…

c9f6303

…NormalizeFieldTransformation.java Co-authored-by: Landon Reed <[email protected]>

refactor(NormalizeFieldTransformation): fix whitespace

499908b

landonreed approved these changes May 26, 2021

View reviewed changes

landonreed merged commit d80f10f into dev May 26, 2021

landonreed deleted the normalize-strings-transformation branch May 26, 2021 20:16

binh-dam-ibigroup mentioned this pull request May 28, 2021

Normalize fields fix #386

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize strings transformation #371

Normalize strings transformation #371

binh-dam-ibigroup commented Mar 22, 2021 •

edited

Loading

codecov-io commented Mar 31, 2021 •

edited

Loading

evansiroky left a comment

evansiroky left a comment

binh-dam-ibigroup commented May 5, 2021

br648 left a comment

br648 May 11, 2021

binh-dam-ibigroup May 11, 2021

Normalize strings transformation #371

Normalize strings transformation #371

Conversation

binh-dam-ibigroup commented Mar 22, 2021 • edited Loading

Checklist

Description

codecov-io commented Mar 31, 2021 • edited Loading

Codecov Report

evansiroky left a comment

Choose a reason for hiding this comment

evansiroky left a comment

Choose a reason for hiding this comment

binh-dam-ibigroup commented May 5, 2021

br648 left a comment

Choose a reason for hiding this comment

br648 May 11, 2021

Choose a reason for hiding this comment

binh-dam-ibigroup May 11, 2021

Choose a reason for hiding this comment

binh-dam-ibigroup commented Mar 22, 2021 •

edited

Loading

codecov-io commented Mar 31, 2021 •

edited

Loading